Fix for #18161 AsyncHttpServer Issue Too many open files. #18198
Conversation
I've been checking the code in asynchttpserver file and found that in the processClient function the connection is never closed after the "while" loop. So even if the keepalive header is off the number of open files is always increasing. With the keepalive off and if the connection is closed after the "while" loop the server never dies. If the header keepalive is on and if it exceeds the pre-defined number of file descriptors it automatically closes the connection! I also added a timeout for keepalive connections.
There was a problem hiding this comment.
This is a good patch when it comes to keep-alive timeout (and it might be good to keep this feature in its own PR and create a new one for the issue you're trying to fix). But you shouldn't just stop client processing when number of active descriptors is too high, that will just lead to more confusion.
You are fixing the keep-alive:off case, right? So why not check for this header and only process one request in that case?
The default value for keepalive timeout is 3600 seconds.
…ons. Disables the keepalive if the active file descriptors exceeds the maxFDs value or if the maximum keepalive timeout is exceeded. In this case, the new connections are closed until the value of the file descriptors drops back down to the maxFDs.
| if (fds > server.maxFDs) or ((now() - startTimeout).inSeconds > server.keepaliveTimeout): | ||
| request.mget().disableKeepalive = true |
There was a problem hiding this comment.
No, you shouldn't do this at all. You should check the headers and see whether keep-alive has been turned off.
There was a problem hiding this comment.
@dom96 can you help me with the code? I want to close the connection when the number of file descriptors is high. Even if the value of keepalive is on I want to close the connection anyway. It is temporary until the number of files descriptors goes down, when the value goes down the connection can remain open. Please help me to solve the issue!
There was a problem hiding this comment.
@mrhdias This isn't what you should do though! Don't close connections when the number of FDs is high, this will just lead to more problems.
|
With the solution I proposed the function shouldAcceptRequest is not necessary. proc serve*(server: AsyncHttpServer, port: Port,
callback: proc (request: Request): Future[void] {.closure, gcsafe.},
address = "";
assumedDescriptorsPerRequest = -1) {.async.} =
listen server, port, address
while true:
#[ This part can be removed and replaced with the one below.
if shouldAcceptRequest(server, assumedDescriptorsPerRequest):
var (address, client) = await server.socket.acceptAddr()
asyncCheck processClient(server, client, address, callback)
else:
poll()
#echo(f.isNil)
#echo(f.repr)
]#
var (address, client) = await server.socket.acceptAddr()
asyncCheck processClient(server, client, address, callback) |
You are fixing the case when "keep-alive" is off. So please focus on that. We can deal with |
The server does not die when the "keep-alive" is off. Dies when is on. If the value of header "connection" == "close" the connection is closed, if it is "keep-alive" it remains open and the number of file descriptors increases and the server die. |
|
@mrhdias this PR targets wrong branch; it should target devel; you don't need to open a new PR for that, you can change the target branch on same PR (read github docs before doing so) |
Thank you @timotheecour I already did it. |
incorrectly though... since conflict files; that's why i suggested to read the github docs prior to doing this. you should:
it should work |
I will create another PR. There are too many conflicts and comments! |
Then there is no problem? When you have keep-alive on and your server receives Right now this is solved by
So what you (or someone else) can do is create two PRs:
|
The server accepts all connections, but temporarily disables the connection "keep-alive", It's the same as having the the "connection" equal to "close" in header.
The shouldAcceptRequest don't solve the issue because I took a look at the asyncdispatch file and I think the activeDescriptors function doesn't read the real number of open file descriptors!
|
I've been checking the code in asynchttpserver file and found that in the processClient function the connection is never closed after the "while" loop. So even if the keepalive header is off the number of open files is always increasing. With the keepalive off and if the connection is closed after the "while" loop the server never dies.
If the header keepalive is on and if it exceeds the pre-defined number of file descriptors it automatically closes the connection! I also added a timeout for keepalive connections.